docs(assets): unify display_name helper docstrings + fixture tests (FE-747)#12399
docs(assets): unify display_name helper docstrings + fixture tests (FE-747)#12399dante01yoon wants to merge 9 commits into
Conversation
…, FE-748) FE-748: drop the isCloud branch in getMediaDisplayName. Resolve through the shared getAssetDisplayFilename helper so the missingMedia surface uses the same fallback chain (user_metadata.filename -> metadata.filename -> display_name -> asset.name) as the asset card and browser surfaces. FE-747: align assetMetadataUtils helper docstrings with the unified display_name shape emitted by both Core and Cloud per the BE-808 Asset Identity RFC. Add fixture tests covering the Cloud (hash + display_name) and OSS (filename + nullable display_name) response shapes.
📝 WalkthroughWalkthroughClarifies asset display helper docs, adds tests ensuring Cloud and OSS assets produce consistent UI labels (including OSS display_name overrides), and refactors missing-media name resolution to use filename-indexed asset lookups with accompanying tests for precedence and library option mapping. ChangesUnified Asset Identity Implementation
Sequence DiagramsequenceDiagram
participant Caller
participant getMediaDisplayName
participant useAssetsStore
participant inputAssetsByFilename
participant getAssetDisplayFilename
Caller->>getMediaDisplayName: call(name)
getMediaDisplayName->>useAssetsStore: get store instance
useAssetsStore-->>getMediaDisplayName: store reference
getMediaDisplayName->>inputAssetsByFilename: lookup(name)
alt Asset found
inputAssetsByFilename-->>getMediaDisplayName: asset
getMediaDisplayName->>getAssetDisplayFilename: getAssetDisplayFilename(asset)
getAssetDisplayFilename-->>getMediaDisplayName: display label
getMediaDisplayName-->>Caller: return label
else Asset not found
inputAssetsByFilename-->>getMediaDisplayName: undefined
getMediaDisplayName-->>Caller: return raw name
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/platform/missingMedia/composables/useMissingMediaInteractions.test.ts (1)
6-12: ⚡ Quick winAvoid module-level mutable mock state; switch to a hoisted mock container.
Using a shared mutable
Mapat module scope makes isolation fragile as this suite grows. Prefer avi.hoistedstate object for the mocked store source of truth.Proposed refactor
-const mockInputAssetsByFilename = new Map<string, AssetItem>() +const mockedAssetsState = vi.hoisted(() => ({ + inputAssetsByFilename: new Map<string, AssetItem>() +})) vi.mock('`@/stores/assetsStore`', () => ({ useAssetsStore: () => ({ - inputAssetsByFilename: mockInputAssetsByFilename + inputAssetsByFilename: mockedAssetsState.inputAssetsByFilename }) })) describe('getMediaDisplayName', () => { beforeEach(() => { - mockInputAssetsByFilename.clear() + mockedAssetsState.inputAssetsByFilename.clear() }) @@ - mockInputAssetsByFilename.set(hash, { + mockedAssetsState.inputAssetsByFilename.set(hash, { @@ - mockInputAssetsByFilename.set(hash, { + mockedAssetsState.inputAssetsByFilename.set(hash, { @@ - mockInputAssetsByFilename.set(hash, { + mockedAssetsState.inputAssetsByFilename.set(hash, { @@ - mockInputAssetsByFilename.set(hash, { + mockedAssetsState.inputAssetsByFilename.set(hash, {As per coding guidelines: "Keep module mocks contained; do not use global mutable state within test files; use vi.hoisted() if necessary."
Also applies to: 22-24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/missingMedia/composables/useMissingMediaInteractions.test.ts` around lines 6 - 12, Replace the module-scoped mutable Map mockInputAssetsByFilename with a hoisted mock container: create a vi.hoisted(() => ({ inputAssetsByFilename: new Map<string, AssetItem>() })) object and have the mocked useAssetsStore return that container's inputAssetsByFilename instead of the top-level Map; update any tests that mutate mockInputAssetsByFilename to mutate the hoisted container (referencing mockInputAssetsByFilename and useAssetsStore in the diff) so each test file run gets isolated, resettable state.src/platform/assets/utils/assetMetadataUtils.test.ts (1)
388-389: ⚡ Quick winRemove redundant inline comments in the new test block.
These comments restate what test names and fixture identifiers already convey; please drop them to match repo guidance on minimizing code comments.
As per coding guidelines: "Avoid new usage of code comments; do not add or retain redundant comments."
Also applies to: 398-399
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/platform/assets/utils/assetMetadataUtils.test.ts` around lines 388 - 389, Remove the redundant inline comments that restate already-clear test names and fixture identifiers — specifically delete the comment text about "Cloud asset: `asset.name` is a content hash; `display_name` carries the user-facing label." (and the similar redundant comments at the other test lines) from the new test block in assetMetadataUtils.test; leave the tests and fixtures unchanged, only remove those comment lines so the tests conform to the "no redundant comments" guideline.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/platform/assets/utils/assetMetadataUtils.test.ts`:
- Around line 388-389: Remove the redundant inline comments that restate
already-clear test names and fixture identifiers — specifically delete the
comment text about "Cloud asset: `asset.name` is a content hash; `display_name`
carries the user-facing label." (and the similar redundant comments at the other
test lines) from the new test block in assetMetadataUtils.test; leave the tests
and fixtures unchanged, only remove those comment lines so the tests conform to
the "no redundant comments" guideline.
In `@src/platform/missingMedia/composables/useMissingMediaInteractions.test.ts`:
- Around line 6-12: Replace the module-scoped mutable Map
mockInputAssetsByFilename with a hoisted mock container: create a vi.hoisted(()
=> ({ inputAssetsByFilename: new Map<string, AssetItem>() })) object and have
the mocked useAssetsStore return that container's inputAssetsByFilename instead
of the top-level Map; update any tests that mutate mockInputAssetsByFilename to
mutate the hoisted container (referencing mockInputAssetsByFilename and
useAssetsStore in the diff) so each test file run gets isolated, resettable
state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4212abbd-b8eb-4827-b0ca-a9f88a461604
📒 Files selected for processing (4)
src/platform/assets/utils/assetMetadataUtils.test.tssrc/platform/assets/utils/assetMetadataUtils.tssrc/platform/missingMedia/composables/useMissingMediaInteractions.test.tssrc/platform/missingMedia/composables/useMissingMediaInteractions.ts
…DisplayName Adds 4 cases against the in-file consumer of getMediaDisplayName that was previously untested: empty-on-missing-widget, Cloud-hash combo values mapped through the shared helper chain (display_name and metadata.filename), OSS filename pass-through, and the candidate-name filter. Caught by the FE-747/748 PR review pass.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #12399 +/- ##
===========================================
- Coverage 77.42% 64.67% -12.75%
===========================================
Files 1629 1514 -115
Lines 111798 77156 -34642
Branches 36754 21779 -14975
===========================================
- Hits 86559 49904 -36655
- Misses 24373 26916 +2543
+ Partials 866 336 -530
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1190 files with indirect coverage changes 🚀 New features to boost your workflow:
|
dosubot
Bot
added
the
size:S
# Conflicts: # src/platform/assets/utils/assetMetadataUtils.test.ts # src/platform/assets/utils/assetMetadataUtils.ts
DrJKL
left a comment
DrJKL
left a comment
There was a problem hiding this comment.
Sunsets really are beautiful, aren't they?
LGTM — single-source-of-truth move onto getAssetDisplayFilename is the right call, the BE-808 cloud/OSS parity tests read cleanly, and the integration test for getLibraryOptions gives real behavioural coverage. A handful of nits below, none blocking.
| if (!isCloud) return name | ||
| return useAssetsStore().getInputName(name) | ||
| const asset = useAssetsStore().inputAssetsByFilename.get(name) | ||
| return asset ? getAssetDisplayFilename(asset) : name |
There was a problem hiding this comment.
Subtle behavioural drift worth flagging in the PR description: the old getInputName(name) path returned asset.name (always the raw asset name). The new getAssetDisplayFilename chain prefers user_metadata.filename → metadata.filename → display_name → asset.name. For Cloud workflows that means the missing-media row and pending-selection label in MissingMediaRow.vue will now surface whatever the queue mapper put into display_name instead of the hash-y asset.name. Almost certainly desired per BE-808, just worth confirming both MissingMediaRow call sites are happy with the richer fallback.
There was a problem hiding this comment.
Confirmed and intended per BE-808. Both MissingMediaRow.vue call sites are happy with the richer fallback — see PR description §1/§2 (live setupState dump) for the per-layer verification. The drift is documented in the Review Focus + Verification sections. No production-visible change today since current Cloud assets have display_name=null (§6); the new layers activate once BE-1043 starts populating it.
| * Human-readable filename for UI labels. | ||
| * Fallback: user_metadata.filename → metadata.filename → display_name → asset.name. | ||
| * For serialized identifiers use {@link getAssetFilename}. | ||
| */ |
There was a problem hiding this comment.
Nit: the trimmed docstring loses the useful "queue output mappers in Cloud populate display_name" context. Since useMissingMediaInteractions.getMediaDisplayName now relies on that contract, consider keeping one short sentence about who populates display_name.
There was a problem hiding this comment.
Intentionally left as-is. The empirical Cloud probe (PR description §6) showed display_name is null across all sampled Cloud assets and asset.name is already a friendly filename — so the "queue output mappers populate display_name" framing is not current production behaviour and was deliberately dropped to avoid a misleading docstring. Per the BE-808 RFC display_name is the unified field both backends emit once BE-1043/1044 land; the docstring reflects that contract rather than naming a specific (currently inaccurate) populator.
|
|
||
| it('honours OSS-emitted display_name when present', () => { | ||
| const ossWithDisplayName: AssetItem = { | ||
| ...ossShape, |
There was a problem hiding this comment.
Nit: name: 'sunset.png' is already inherited from ...ossShape — the explicit re-assignment is a no-op and slightly muddies what the test is varying (the display_name field). Safe to drop.
There was a problem hiding this comment.
Done — dropped the redundant re-assignment; the fixture now only varies display_name.
| useMissingMediaInteractions | ||
| } from '@/platform/missingMedia/composables/useMissingMediaInteractions' | ||
|
|
||
| const mockInputAssetsByFilename = new Map<string, AssetItem>() |
There was a problem hiding this comment.
Nit: module-level mutable mockInputAssetsByFilename is the "global mutable state in test file" pattern the unit-testing guide steers away from. clear() in beforeEach makes it safe in practice, but vi.hoisted(() => ({ inputAssetsByFilename: new Map() })) would match the project convention and make the hoisting explicit.
There was a problem hiding this comment.
Done — switched to a single vi.hoisted(() => ({ inputAssetsByFilename, getNodeByExecutionId, resolveComboValues })) container in f0299e5, matching the project convention.
| mockGetNodeByExecutionId(...args) | ||
| })) | ||
|
|
||
| const mockResolveComboValues = vi.fn() |
There was a problem hiding this comment.
Nit: same hoisting story for mockGetNodeByExecutionId / mockResolveComboValues. Works because the inner refs are only dereferenced when the mocked fns are called, but vi.hoisted() removes the latent TDZ trap and signals intent to the next editor.
There was a problem hiding this comment.
Done — getNodeByExecutionId / resolveComboValues now live in the same hoisted container and are assigned directly into the mock factories, removing the module-level vi.fn() consts.
| pendingSelection: {}, | ||
| uploadState: {}, | ||
| missingMediaCandidates: null, | ||
| removeMissingMediaByName: vi.fn() |
There was a problem hiding this comment.
Nit: removeMissingMediaByName: vi.fn() (and updateInputs, addAlert further down) are wired up but never asserted. Either drop them from the mocks, or add a confirmSelection / handleUpload test that actually exercises them.
There was a problem hiding this comment.
Dropped updateInputs / removeMissingMediaByName from the mocks — the tested paths (getMediaDisplayName, getLibraryOptions) never call them. Kept the toastStore module mock as a needed boundary. A dedicated confirmSelection / handleUpload spec is a worthwhile follow-up but out of scope here.
- Hoist mutable mock state via vi.hoisted to match test conventions - Drop unused store mock fns (updateInputs, removeMissingMediaByName) - Remove redundant name re-assignment in OSS display_name fixture
…lsts-helpers-to-unified-asset
…lsts-helpers-to-unified-asset
…igrate-assetmetadatautilsts-helpers-to-unified-asset # Conflicts: # src/platform/assets/utils/assetMetadataUtils.ts # src/platform/missingMedia/composables/useMissingMediaInteractions.ts
dante01yoon
dismissed stale reviews from DrJKL and coderabbitai[bot]
via
c52dcfc
dante01yoon
changed the title
🎨 Storybook: ✅ Built — View Storybook🎭 Playwright: ✅ 1687 passed, 0 failed · 2 flaky📊 Browser Reports
📦 Bundle: 7.54 MB gzip 🟢 -27 BDetailsSummary
Category Glance App Entry Points — 46.9 kB (baseline 46.9 kB) • ⚪ 0 BMain entry bundles and manifests
Status: 1 added / 1 removed Graph Workspace — 1.25 MB (baseline 1.25 MB) • ⚪ 0 BGraph editor runtime, canvas, workflow orchestration
Status: 1 added / 1 removed Views & Navigation — 97.7 kB (baseline 97.7 kB) • ⚪ 0 BTop-level views, pages, and routed surfaces
Status: 9 added / 9 removed / 3 unchanged Panels & Settings — 512 kB (baseline 512 kB) • ⚪ 0 BConfiguration panels, inspectors, and settings screens
Status: 11 added / 11 removed / 15 unchanged User & Accounts — 26.9 kB (baseline 26.9 kB) • ⚪ 0 BAuthentication, profile, and account management bundles
Status: 7 added / 7 removed / 3 unchanged Editors & Dialogs — 112 kB (baseline 112 kB) • ⚪ 0 BModals, dialogs, drawers, and in-app editors
Status: 4 added / 4 removed / 1 unchanged UI Components — 65.2 kB (baseline 65.2 kB) • ⚪ 0 BReusable component library chunks
Status: 5 added / 5 removed / 9 unchanged Data & Services — 269 kB (baseline 269 kB) • ⚪ 0 BStores, services, APIs, and repositories
Status: 13 added / 13 removed / 3 unchanged Utilities & Hooks — 3.35 MB (baseline 3.35 MB) • 🟢 -173 BHelpers, composables, and utility bundles
Status: 17 added / 17 removed / 15 unchanged Vendor & Third-Party — 15.3 MB (baseline 15.3 MB) • ⚪ 0 BExternal libraries and shared vendor chunks Status: 16 unchanged Other — 10.7 MB (baseline 10.7 MB) • ⚪ 0 BBundles that do not match a named category
Status: 67 added / 67 removed / 95 unchanged ⚡ Performance
|
mattmillerai
added
the
cursor-review
…igrate-assetmetadatautilsts-helpers-to-unified-asset # Conflicts: # src/platform/assets/utils/assetMetadataUtils.test.ts
| * Human-readable filename for UI labels. | ||
| * Fallback: user_metadata.filename → metadata.filename → display_name → asset.name. | ||
| * For serialized identifiers use {@link getAssetFilename}. | ||
| */ |
There was a problem hiding this comment.
suggestion: (non-blocking) the shortened docstring drops "Use this helper for labels/titles only" — keeping that positive constraint alongside the existing cross-reference to getAssetFilename would make the display-vs-identifier distinction clear without needing to follow the link.
| * `asset.name` is a content hash. Use this helper for labels/titles only; | ||
| * for serialized identifiers use {@link getAssetFilename}. | ||
| * Human-readable filename for UI labels. | ||
| * Fallback: user_metadata.filename → metadata.filename → display_name → asset.name. |
There was a problem hiding this comment.
nitpick: (non-blocking) every other export in this file opens with an imperative verb (Gets the..., Extracts..., Resolves...) — the new noun-phrase opening "Human-readable filename for UI labels." breaks that consistency.
| id: 'oss-asset-id', | ||
| name: 'sunset.png', | ||
| hash: null, | ||
| display_name: undefined |
There was a problem hiding this comment.
nitpick: (non-blocking) ossShape sets hash: null but none of the three new tests exercise getAssetStoredFilename or getAssetUrlFilename, which are the only functions that consume hash. The field is inert here — either add a stored-filename assertion or drop it to avoid implying coverage that is not there.
| }) | ||
|
|
||
| it('honours OSS-emitted display_name when present', () => { | ||
| const ossWithDisplayName: AssetItem = { |
There was a problem hiding this comment.
nitpick: (non-blocking) "honours OSS-emitted display_name when present" exercises the same branch as the existing "falls back to display_name when filename metadata is absent" test at line 365 — identical code path, different fixture string. Per the project parsimony guideline this could be folded into the existing suite or dropped.
| * Human-readable filename for UI labels. | ||
| * Fallback: user_metadata.filename → metadata.filename → display_name → asset.name. | ||
| * For serialized identifiers use {@link getAssetFilename}. | ||
| */ |
There was a problem hiding this comment.
suggestion: (non-blocking) the shortened docstring drops "Use this helper for labels/titles only" — keeping that positive constraint alongside the existing cross-reference to getAssetFilename would make the display-vs-identifier distinction clear without needing to follow the link.
| * `asset.name` is a content hash. Use this helper for labels/titles only; | ||
| * for serialized identifiers use {@link getAssetFilename}. | ||
| * Human-readable filename for UI labels. | ||
| * Fallback: user_metadata.filename → metadata.filename → display_name → asset.name. |
There was a problem hiding this comment.
nitpick: (non-blocking) every other export in this file opens with an imperative verb (Gets the..., Extracts..., Resolves...) — the new noun-phrase opening "Human-readable filename for UI labels." breaks that consistency.
| id: 'oss-asset-id', | ||
| name: 'sunset.png', | ||
| hash: null, | ||
| display_name: undefined |
There was a problem hiding this comment.
nitpick: (non-blocking) ossShape sets hash: null but none of the three new tests exercise getAssetStoredFilename or getAssetUrlFilename, which are the only functions that consume hash. The field is inert here — either add a stored-filename assertion or drop it to avoid implying coverage that is not there.
| }) | ||
|
|
||
| it('honours OSS-emitted display_name when present', () => { | ||
| const ossWithDisplayName: AssetItem = { |
There was a problem hiding this comment.
nitpick: (non-blocking) "honours OSS-emitted display_name when present" exercises the same branch as the existing "falls back to display_name when filename metadata is absent" test at line 365 — identical code path, different fixture string. Per the project parsimony guideline this could be folded into the existing suite or dropped.
| * Human-readable filename for UI labels. | ||
| * Fallback: user_metadata.filename → metadata.filename → display_name → asset.name. | ||
| * For serialized identifiers use {@link getAssetFilename}. | ||
| */ |
There was a problem hiding this comment.
suggestion: (non-blocking) the shortened docstring drops "Use this helper for labels/titles only" — keeping that positive constraint alongside the existing cross-reference to getAssetFilename would make the display-vs-identifier distinction clear without needing to follow the link.
| * `asset.name` is a content hash. Use this helper for labels/titles only; | ||
| * for serialized identifiers use {@link getAssetFilename}. | ||
| * Human-readable filename for UI labels. | ||
| * Fallback: user_metadata.filename → metadata.filename → display_name → asset.name. |
There was a problem hiding this comment.
nitpick: (non-blocking) every other export in this file opens with an imperative verb (Gets the..., Extracts..., Resolves...) — the new noun-phrase opening "Human-readable filename for UI labels." breaks that consistency.
| id: 'oss-asset-id', | ||
| name: 'sunset.png', | ||
| hash: null, | ||
| display_name: undefined |
There was a problem hiding this comment.
nitpick: (non-blocking) ossShape sets hash: null but none of the three new tests exercise getAssetStoredFilename or getAssetUrlFilename, which are the only functions that consume hash. The field is inert here — either add a stored-filename assertion or drop it to avoid implying coverage that is not there.
| }) | ||
|
|
||
| it('honours OSS-emitted display_name when present', () => { | ||
| const ossWithDisplayName: AssetItem = { |
There was a problem hiding this comment.
nitpick: (non-blocking) "honours OSS-emitted display_name when present" exercises the same branch as the existing "falls back to display_name when filename metadata is absent" test at line 365 — identical code path, different fixture string. Per the project parsimony guideline this could be folded into the existing suite or dropped.
christian-byrne
assigned
dante01yoon and unassigned
christian-byrne,
DrJKL,
synap5e and
mattmillerai
5 participants
Summary
Originally bundled FE-747 + FE-748. After merging latest
main, FE-748 is obsolete (see below) and nothing of it remains, so this PR is now FE-747 only: theassetMetadataUtilsdisplay helpers are documented against the unified BE-808 asset shape, backed by Cloud/OSS fixture tests. No production behavior change — only docstrings and tests; the helper bodies are byte-identical tomain.Why FE-748 is obsolete
FE-748 migrated
getMediaDisplayNameinsrc/platform/missingMedia/composables/useMissingMediaInteractions.tsonto the unifieddisplay_name. Upstream PR #12705 ("Simplify missing media error presentation") then removed the entire missing-media library/upload flow, deleting that composable along withMissingMediaRow.vueandMissingMediaLibrarySelect.vue.getMediaDisplayName/getLibraryOptionsnow have zero consumers anywhere insrc/— the missing-media error tab no longer resolves asset display names at all; it renders a flat{nodeName} - {inputName}list. FE-748 has no surface left to land on. The merge conflict was resolved by accepting the upstream deletion and removing the orphaned spec; FE-748 is closed as superseded.Changes (FE-747)
src/platform/assets/utils/assetMetadataUtils.tsgetAssetDisplayFilename/getAssetCardTitledocstrings:display_nameis the unified user-facing label per the BE-808 Asset Identity RFC, not a "Cloud-only / queue-output mapper" field. No code change — the helpers already consumedisplay_namethrough the existing fallback chain. The merge also preservedgetAssetStoredFilename(added onmain).src/platform/assets/utils/assetMetadataUtils.test.tsasset.name= hash,display_name= filename) and OSS (asset.name= filename, nullabledisplay_name) — asserting both helpers render the same label, plusdisplay_nameprecedence cases.Verification — 2026-06-17 (CDP, OSS + Cloud)
Dynamic-imported the merged module and ran the helpers against real data in both environments:
--enable-assets) behind the dev server: real input assets (04_toolbox.png,image64x64.webp, …) resolve to their friendly filename; the Imported-tab cards render filenames, never hashes.cloud.comfy.orgsession, real/api/assets: correct labels forinput/output/models. Key case — anoutputasset whosehashis a content hash (183f9635….mp4) resolves to itsdisplay_name(video/ComfyUI-vid_3_00001_.mp4), no hash leak. Models resolve viauser_metadata.filename; the card title uses the curatedmetadata.name(pre-existing behavior).user_metadata.filename → metadata.filename → display_name → asset.nameconfirmed.display_name(BE-1043 has landed), so the fallback chain is live, not dormant. No regression in either environment.pnpm typecheck,eslint, the 59assetMetadataUtils.test.tscases, and full CI (incl. cloud Playwright) pass.Refs
display_name, now live)